C#: Data flow performance improvements#1058
Conversation
Before this change, ``` flowOutOfCallableStep(CallNode call, ReturnNode ret, OutNode out, CallContext cc) ``` would compute all combinations of call sites `call` and returned expressions `ret` up front. Now, we instead introduce explicit return nodes, so each callable has exactly one return node (as well as one for each `out`/`ref` parameter). There is then local flow from a returned expression to the relevant return node, and `flowOutOfCallableStep()` computes combinations of call sites and return nodes. Not only does this result in better performance, it also makes `flowOutOfCallableStep()` symmetric to `flowIntoCallableStep()`, where each argument is mapped to a parameter, and not to all reads of that parameter.
calumgrant
left a comment
There was a problem hiding this comment.
Awesome speedup. A few minor questions.
| node instanceof ParameterNode or | ||
| node instanceof OutNode | ||
| node instanceof OutNode or | ||
| node instanceof NormalReturnNode or |
There was a problem hiding this comment.
Just node instanceof ReturnNode ?
| node instanceof ReturnNode | ||
| or | ||
| exists(ReturnNode rn | localFlowStep(node, rn, config) | | ||
| rn instanceof NormalReturnNode or |
There was a problem hiding this comment.
I don't see how the body of this exists can be false? So perhaps
localFlowStep(node, any(ReturnNode rn), config)
|
|
||
| override DotNet::Callable getEnclosingCallable() { this = TNormalReturnNode(result) } | ||
|
|
||
| override string toString() { result = "return" } |
There was a problem hiding this comment.
"return " + getEnclosingCallable().getName()
Could that be useful?
| @@ -591,18 +581,47 @@ module DataFlow { | |||
| abstract private class ReturnNode extends Node { } | |||
There was a problem hiding this comment.
Could it be useful to have getAReturnedExpr() on this class?
There was a problem hiding this comment.
It is an internal class, so I think we should only add it if we need it later on.
631c7b0 to
8959d52
Compare
calumgrant
left a comment
There was a problem hiding this comment.
Wait for dist-compare results.
The new report is ready (internal link), and things still look good. |
This PR has a quite positive impact on performance (internal link), as the default LGTM suite contains many data-flow-based queries.
Commit-by-commit review is encouraged.